Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create_application_commands can now take any IntoIterator: not just &[] #298

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

zkxs
Copy link

@zkxs zkxs commented Aug 12, 2024

This is a small change: currently poise::builtins::create_application_commands takes a &[crate::Command<U, E>]. This PR changes it to take an IntoIterator<Item=&crate::Command<U, E>>. This doesn't really change much, except it makes the API slightly easier to work with if API users don't simply have a Vec<Command<U, E> lying around: for example if you're dynamically computing which commands should be set on a guild-by-guild basis.

@GnomedDev
Copy link
Member

This is a breaking change, therefore needs to be targeted at the next branch.

@zkxs zkxs changed the base branch from current to next August 12, 2024 09:43
@zkxs zkxs force-pushed the create_application_commands_IntoIterator branch 2 times, most recently from 60f2a85 to 28b9b64 Compare August 12, 2024 09:50
@zkxs zkxs force-pushed the create_application_commands_IntoIterator branch from 28b9b64 to b45fe30 Compare August 12, 2024 09:53
@zkxs
Copy link
Author

zkxs commented Aug 12, 2024

@GnomedDev after some confusion between the serenity-next and next branches I've changed the base.

@GnomedDev
Copy link
Member

Do you want to also update the functions that call create_application_commands to take advantage of the more permissive function signature?

@zkxs
Copy link
Author

zkxs commented Aug 12, 2024

Sure; it should be done now unless I missed any: it looks like it's just register_in_guild and register_globally.

@GnomedDev
Copy link
Member

Ignore the clippy issues, they are failing for unrelated issues, while you fix the rustfmt issue could you use Iterator::size_hint to restore the Vec::with_capacity optimization?

@zkxs
Copy link
Author

zkxs commented Aug 12, 2024

@GnomedDev Should be done. I'm not checking the upper bound on size_hint, but that seems correct to me because in my experience overestimating the allocation tends to hurt performance more than underestimating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants